WIP: Implement h2 protocol #1026

Vibhu-Agarwal · 2021-04-30T22:08:05Z

Closes #47

HTTP Parsing

Using h2

PR #214

It was great! I've picked a lot of stuff from there.
I've mostly worked on extending the PR, removing bugs and modernizing the code.
I've opted to omit a few implementations though, on which I'd like to have some discussion.

Motivation from Hypercorn's code

I also read the hypercorn's code, which has the implementation for HTTP/2 already. I got a pretty good idea of http2's working from there.

priority

It uses priority to build a priority tree and manage the streams.
I haven't yet implemented this in this PR (as it wasn't, in #214). Should we use it?
If we use it, it will also help in implementing code for handling h2 events like WindowUpdated, PriorityUpdated and RemoteSettingsChanged (which are currently ignored).

stream_buffers

Hypercorn also uses StreamBuffers to handle the data of different streams per connection. This may be due to the fact that h2 doesn't buffer data internally for sending. I'm not sure though, as hypercorn checks the chunk size (before sending) anyway.
I haven't yet implemented this in this PR (as it wasn't, in #214). Should we use it?

Server Push

I wasn't sure if I should go ahead with HTTP Server-Push's implementation or not, as its status is not clear.
Should we support it?

In this PR

Some h2-specific changes in SSL-Context

uvicorn/uvicorn/config.py

Line 215 in a8dcf14

self.h2_ssl_ciphers = "ECDHE+AESGCM"

uvicorn/uvicorn/config.py

Lines 116 to 125 in a8dcf14

    
           if enable_h2: 
        
               ctx.options |= ( 
        
                   ssl.OP_NO_SSLv2 
        
                   | ssl.OP_NO_SSLv3 
        
                   | ssl.OP_NO_TLSv1 
        
                   | ssl.OP_NO_TLSv1_1 
        
                   | ssl.OP_NO_COMPRESSION 
        
                   | ssl.OP_CIPHER_SERVER_PREFERENCE 
        
               ) 
        
               ctx.set_alpn_protocols(["h2", "http/1.1"])

No websocket support, yet.

Unlike h11, a Connection: Upgrade (to webocket) request raises h2.exceptions.ProtocolError here:

uvicorn/uvicorn/protocols/http/h2_impl.py

Lines 219 to 221 in b683626

    
           try: 
        
               events = self.conn.receive_data(data) 
        
           except h2.exceptions.ProtocolError:

This can be resolved through the usage of Extended CONNECT Method.

uvicorn/uvicorn/protocols/http/h2_impl.py

Line 111 in b683626

h2.settings.SettingCodes.ENABLE_CONNECT_PROTOCOL: 1,

That will have to be handled here:

uvicorn/uvicorn/protocols/http/h2_impl.py

Lines 284 to 285 in b683626

    
           if self.scope["method"] == "CONNECT" and websocket_protocol: 
        
               # TODO: Websocket Extended CONNECT Implementation

Tests

For tests for config.py,
- It didn't have much to cover up the coverage, but SSL-things had to be tested. So, one for that.
For tests for h11_impl.py,
- These two upgrades need tests:
  1. h2c-upgrade
  2. H2-Prior-Knowledge Connection-Preface
- Work: I added one for both of them (got the coverage; but checks in response still remain):
  1. h2c-upgrade: I added one for a simple GET request.
    For some reason, test_h2c_upgrade_request is causing failure of some un-related tests in Python3.8 and Python3.9 (runs fine in Python <=3.7). Commit 81f21be
    Manual-Testing: I'm not sure how to manually test this one.
  2. H2-Prior-Knowledge Connection-Preface: The test is really a minimal one, not even hitting any endpoint. So I'm not even sure if this is sufficient or not.
    Manual-Testing: I tried this with a POST request using curl. It works. I'm not sure how to replicate it in Python or even get raw HTTP request out of it.
- Basically, I'm out of ideas to implement tests for these.
For tests for h2_impl.py,
- Just like HTTPToolsProtocol and H11Protocol, H2Protocol needs testing for a lot of stuff.
- I saw two options of implementation methods:
  1. test_http.py way (using mocks). I tried this out (had to modify get_connected_protocol() a bit) but got messed up with valid raw HTTP/2 requests and SSL-Context passing. Couldn't debug and get it to work.
  2. test_ssl.py way (using httpx). I installed Starlette, added a (very) small app, and tried out a test with a simple POST request. It kinda worked out. So I've added this one in the PR.
- Manual-Testing: I tried with my (Firefox) browser using a sane app on simple GET requests. It seems to work fine.
  I haven't tried broken apps and others with exceptions.

WIP

I've marked a lot of places in the code with # TODO: Task and added some comments for decision-related discussion or missing implementation.
I've also tried to keep the code of h2_impl.py similar to httptools_impl.py and h11_impl.py.

Help Needed

I'm just giving it a go. Hope to complete this soon 🤞
Will need reviews, inputs, tests, ideas and a lot of help from everyone 🤗

Merge "encode:master" into h2_impl

Vibhu-Agarwal · 2021-06-01T23:29:11Z

^ So that some discussion could be started.
The Work is still In-Progress though.

# Resolved Conflicts: # tests/test_config.py # uvicorn/config.py # uvicorn/protocols/http/h11_impl.py

Kludex · 2021-06-29T22:12:51Z

requirements.txt

@@ -20,6 +20,7 @@ types-pyyaml
 trustme
 cryptography
 coverage
+starlette


Do we need starlette? What's the idea here?

Kludex · 2021-06-29T22:13:51Z

setup.py

@@ -47,6 +47,7 @@ def get_packages(package):
    "asgiref>=3.3.4",
    "click>=7.*",
    "h11>=0.8",
+    "h2>=4.0.0",


We want to do the same as httpx: https://github.com/encode/httpx/blob/ab64f7c41fc0fbe638dd586fecf0689c847109bb/setup.py#L66

Kludex · 2021-06-29T22:17:38Z

uvicorn/protocols/http/h2_impl.py

+                # TODO: Implement or Not?
+                # https://groups.google.com/a/chromium.org/g/blink-dev/c/K3rYLvmQUBY/m/vOWBKZGoAQAJ 😕  # noqa: E501


We should, yep.

tomchristie · 2021-06-30T09:57:34Z

So, I'm neither necessarily for or against this, but I do have an alternate perspective that I think is worth sharing/considering.

See the discussion in... #1105

tomchristie · 2021-06-30T10:00:18Z

It may well make sense for us to deliberately limit the scope of Uvicorn to being an HTTP/1.1 server.
The tighter the scope, the more focused our resources can be on making sure we do a fantastic job in a particular space.

Kludex · 2022-01-06T11:42:12Z

@Vibhu-Agarwal Thanks for the effort here, but I think by #1105 we'll highlight hypercorn as the recommendation for HTTP/2.

Sorry for taking so long to act here.

Vibhu-Agarwal added 30 commits April 27, 2021 20:52

Updated gitignore

65dff56

Added (empty) h2_impl.py

700046c

Added some code ... till RequestReceived event

773415e

Set basic triggers for event-handlers

159dd3b

Fix on_request_received

fef0bea

Added remaining methods of H2Protocol

b92c359

Added code for RequestResponseCycle

1c5d053

Added some boilerplate for handle_upgrade in h11_impl.py

ec4b786

Handling h2c in h11_impl.py (TODO: Similarly in httptools)

d2f329e

Handling connection preface in h11_impl.py

086cc29

Modify method prototype: H2Protocol - connection_made()

eb12592

Fixed connection preface check in h11_impl.py

d238f26

Added comments regarding missing implementations

e694792

Reverted .gitignore

00dce01

Added comments regarding missing implementations

c03b6e8

Applied black (linting)

293aaf6

Reformatted h2_impl.py

2a6f2dc

python -m cli_tools.usage

8f6a825

Added h2 in requirements

394140a

Added h2-specific SSLContext options in create_ssl_context()

5d6dd1b

Added h2-connection local settings (through config)

7e96009

h2c handling implementation added in h2_impl.py

3b5c906

Added ECDHE+AESGCM as SSL-Cipher-Suite for h2

a8dcf14

Added TODO for Websocket Extended CONNECT

b683626

Merge branch 'master' into h2_impl

444a359

Refactor w.r.t. encode#1034

4d4de11

Merge branch 'encode:master' into h2_impl

fb90043

Merge 'encode:master' into h2_impl

1b2c1f2

Merge "encode:master" into h2_impl

dbfa251

Merge "encode:master" into h2_impl

Remove irrelevant return statement

a0deef4

Vibhu-Agarwal added 10 commits May 30, 2021 22:46

Merge branch 'encode:master' into h2_impl

301c171

tests: add coverage for create_ssl_context [config.py]

96fe3d7

Merge branch 'encode:master' into h2_impl

b16564b

Fix h2c bug in h11_impl.py

e7e00db

Refactor w.r.t. encode#869

0e7df0a

Fix bugs (similar to ones in previous commit)

59c95bf

Merge branch 'encode:master' into h2_impl

f7e5b8b

auto upgrade (h11=>h2), when alpn_protocol == "h2"

7cae838

Added a TODO for a probable implementation done in encode#929

56140b9

Added a couple of EMPTY tests (h2c & PRI-h2)

81f21be

Vibhu-Agarwal marked this pull request as ready for review June 1, 2021 23:26

Vibhu-Agarwal added 10 commits June 3, 2021 03:02

Added test_h2.py with a simple POST request

0e6ea51

Remove test_h2c_upgrade_request from test_http.py

ad3f35f

Fix the STUPID bug that has been freakin me out

f4665bb

Please fix!

49f251a

Remove "handle_upgrade()" from h2_impl.py

2741e0f

on the way to fix h2c

df02d8c

Merge branch 'encode:master' into h2_impl

1e4bdd9

Merge branch 'encode:master' into h2_impl_mm1

4766009

# Resolved Conflicts: # tests/test_config.py # uvicorn/config.py # uvicorn/protocols/http/h11_impl.py

Merge branch 'encode:master' into h2_impl

59d663a

Merge branch 'encode:master' into h2_impl

919d3f3

Kludex reviewed Jun 29, 2021

View reviewed changes

Kludex closed this Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Implement h2 protocol #1026

WIP: Implement h2 protocol #1026

Vibhu-Agarwal commented Apr 30, 2021 •

edited

Loading

Vibhu-Agarwal commented Jun 1, 2021 •

edited

Loading

Kludex Jun 29, 2021

Kludex Jun 29, 2021

Kludex Jun 29, 2021

tomchristie commented Jun 30, 2021

tomchristie commented Jun 30, 2021

Kludex commented Jan 6, 2022

	if enable_h2:
	ctx.options \|= (
	ssl.OP_NO_SSLv2
	\| ssl.OP_NO_SSLv3
	\| ssl.OP_NO_TLSv1
	\| ssl.OP_NO_TLSv1_1
	\| ssl.OP_NO_COMPRESSION
	\| ssl.OP_CIPHER_SERVER_PREFERENCE
	)
	ctx.set_alpn_protocols(["h2", "http/1.1"])

	try:
	events = self.conn.receive_data(data)
	except h2.exceptions.ProtocolError:

	if self.scope["method"] == "CONNECT" and websocket_protocol:
	# TODO: Websocket Extended CONNECT Implementation

		# TODO: Implement or Not?
		# https://groups.google.com/a/chromium.org/g/blink-dev/c/K3rYLvmQUBY/m/vOWBKZGoAQAJ 😕 # noqa: E501

WIP: Implement h2 protocol #1026

WIP: Implement h2 protocol #1026

Conversation

Vibhu-Agarwal commented Apr 30, 2021 • edited Loading

HTTP Parsing

PR #214

Motivation from Hypercorn's code

priority

stream_buffers

Server Push

In this PR

Tests

WIP

Help Needed

Vibhu-Agarwal commented Jun 1, 2021 • edited Loading

Kludex Jun 29, 2021

Choose a reason for hiding this comment

Kludex Jun 29, 2021

Choose a reason for hiding this comment

Kludex Jun 29, 2021

Choose a reason for hiding this comment

tomchristie commented Jun 30, 2021

tomchristie commented Jun 30, 2021

Kludex commented Jan 6, 2022

Vibhu-Agarwal commented Apr 30, 2021 •

edited

Loading

Vibhu-Agarwal commented Jun 1, 2021 •

edited

Loading